Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use first non-empty Nav post as primary fallback for Nav block #36740

Merged
merged 16 commits into from Nov 24, 2021
Merged

Use first non-empty Nav post as primary fallback for Nav block #36740

merged 16 commits into from Nov 24, 2021

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Nov 22, 2021 •
edited

Description

In #36724 we added the Page List block as a fallback for when the Nav block has no menu selected.

This PR iterates on that idea by preferring to use the first non-empty Navigation (i.e. wp_navigation Post) as the primary fallback. Only if a suitable Navigation Post is not found do we fallback (again!) to the Page List.

This aims to mirror how the fallback mechanic for wp_nav_menu works currently in Core (as described in the Issue).

Closes #36721

How has this been tested?

Testing is as follows:

### Preparation

  • Before testing use Fakerpress Plugin (or similar) to add a load of pages to your site. Some should be nested parent/child.
  • Also use the Nav block to create one or two Menus on your site being sure to add some items. Make sure they are saved as Navigation post types (check by going to Appearance -> Navigation).

Testing

  • Open Site Editor.
  • Remove any existing Nav block.
  • Add Nav block from scratch. Don't add any items!
  • Save your empty Nav block and publish the post.
  • Go to front of site.
  • See that the first of your Navigation Menus is rendered by default even though no Menu has been selected.
  • Remove all of the items from the Navigation Post that is currently being used as the fallback - this should make it ineligible to be used as a fallback.
  • Go to front of site. Check that a different Navigation Post is being used as a fallback.
  • Remove all your Navigation Posts - that's right, ditch them all!
  • Go to front of site. Check that it falls back to the Page List.

Screenshots

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@getdave getdave added the [Block] Navigation Affects the Navigation Block label Nov 22, 2021
@getdave getdave self-assigned this Nov 22, 2021
@getdave getdave added this to 👀 PRs needing review in Navigation block via automation Nov 22, 2021
@getdave getdave mentioned this pull request Nov 22, 2021
Closed
talldan
talldan reviewed Nov 23, 2021
View reviewed changes
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far.

packages/block-library/src/navigation/index.php Outdated Show resolved Hide resolved
packages/block-library/src/navigation/index.php Outdated Show resolved Hide resolved
packages/block-library/src/navigation/index.php Outdated Show resolved Hide resolved
@getdave getdave mentioned this pull request Nov 23, 2021
Merged
7 tasks
@talldan talldan added the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 23, 2021
getdave added a commit that referenced this pull request Nov 23, 2021
@getdave
Remove code duplication
0e8fbc6 
Addresses #36740 (comment)
@getdave getdave requested a review from talldan November 23, 2021 11:45
@carolinan
Copy link
Contributor

I did a manual test by following the test instructions and did not see any issues. I did not do a code review.

@getdave getdave force-pushed the try/first-menu-as-nav-block-fallback branch from 58ae500 to 2831ff3 Compare November 23, 2021 15:46
getdave and others added 2 commits November 24, 2021 12:54
@getdave @adamziel
Limit Nav Posts query to a single item
ead6d3c 
Co-authored-by: Adam Zielinski <adam@adamziel.com>
@getdave
Fix linting
b0c80aa
adamziel
adamziel approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@adamziel adamziel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few suggestions related to naming, but otherwise this looks good to me! If you added a test or two, that would be absolutely stellar. Otherwise, 🚢

@getdave @adamziel
Improve function naming
4dbb220 
Co-authored-by: Adam Zielinski <adam@adamziel.com>
@getdave getdave mentioned this pull request Nov 24, 2021
Open
@getdave
Copy link
Contributor Author

getdave commented Nov 24, 2021

I left a few suggestions related to naming, but otherwise this looks good to me! If you added a test or two, that would be absolutely stellar. Otherwise, 🚢

Let's ship and I'll follow up tomorrow with e2e tests. I'll also sort out the Page List block attribute vs context thing.

@getdave getdave merged commit 6ddf0fc into trunk Nov 24, 2021
Navigation block automation moved this from 👀 PRs needing review to ✅ Done Nov 24, 2021
@getdave getdave deleted the try/first-menu-as-nav-block-fallback branch November 24, 2021 16:04
@github-actions github-actions bot added this to the Gutenberg 12.1 milestone Nov 24, 2021
@bph
Copy link
Contributor

bph commented Nov 24, 2021

@getdave I did s brief run through this before it was merged but didn't get to add my comments before it go merged.
You had great test instructions! Thank you.

After this sequence:

  • Open Site Editor.
  • Remove any existing Nav block.
  • Add Nav block from scratch. Don't add any items!
  • Save your empty Nav block and publish the post.
  • Go to front of site.
  • See that the first of your Navigation Menus is rendered by default even though no Menu has been selected.

I did see the only menu I had was rendered. Putting myself in the shoes of a site owner or builder. I saw the menu, saw it had the wrong pages in there, so I clicked on the Edit Site link on top of my screen. When the site editor loaded, I didn't know what to do next. My expectation would have been to see the menu now also in the editor, so I can make the changes, I wanted to make.

I do like the default behavior. I just not sure what my expectation tells me about what happens next. Maybe this would work?

Once the default menu is rendered on the frontend, because none was assigned before, that on subsequent Site editor views it could also be rendered there?

Second observation:
After I emptied all menus and trash them I didn't see a navigation rendered on the Front end. That worked. I looked into the Developer Tools and I saw a bunch of navigation html, though
Screen Shot 2021-11-24 at 10 58 53 AM

As if there is still an invisible navigation block.
I didn't dive into this yet further, it could come from the theme template, though.

@getdave
Copy link
Contributor Author

getdave commented Nov 25, 2021

Thanks @bph - appreciate you taking time to test this. I'll double check we can still fallback to Page List and the behaviour around what happens if Page List isn't available.

In terms of the UX we could show a warning on the block that it's currently showing a placeholder on the front end. I'm not confident we could land that in time for 5.9 though given everything else that's still outstanding.

We've also received some feedback / concerns about the fallback situation. I'd like to see how that plays out during the Beta period. It's simple enough that, if we find the majority of folks are not onboard with that, we can revert to simply rendering nothing.

Thanks again 🙇

This was referenced Nov 25, 2021
Merged
Merged
@noisysocks noisysocks removed the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 28, 2021
noisysocks pushed a commit that referenced this pull request Nov 29, 2021
@getdave @adamziel @noisysocks
Use first non-empty Nav post as primary fallback for Nav block (#36740)
81fb420 
* Use non-empty Nav post as fallback

* Remove code duplication

Addresses #36740 (comment)

* Extract finding non empty nav block to function

* Extract process of getting fallback to dedicated function

* Apply block prefix to functions

* Add additional safety check around parsing Nav blocks

* Extract function for removing null blocks and apply

* Check for empty parsed Nav blocks

* Improve function comment

Addresses #36740 (comment)

* Amend ordering params to approximate current wp_nav_menus behaviour

* Fetch Nav posts in query rather than filtering in memory

Implements suggestion from #36740 (comment)

* Improve comments

* Spacing

* Limit Nav Posts query to a single item

Co-authored-by: Adam Zielinski <adam@adamziel.com>

* Fix linting

* Improve function naming

Co-authored-by: Adam Zielinski <adam@adamziel.com>

Co-authored-by: Adam Zielinski <adam@adamziel.com>
@noisysocks noisysocks mentioned this pull request Nov 29, 2021
Closed
@femkreations femkreations mentioned this pull request Apr 15, 2022
Closed
82 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacedmonkey spacedmonkey spacedmonkey left review comments

@noisysocks noisysocks noisysocks left review comments

@adamziel adamziel adamziel approved these changes

@anton-vlasenko anton-vlasenko Awaiting requested review from anton-vlasenko

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

@talldan talldan Awaiting requested review from talldan

@draganescu draganescu Awaiting requested review from draganescu

Assignees

@getdave getdave

Labels
[Block] Navigation Affects the Navigation Block
Projects
Navigation block
✅ Done
Milestone
Gutenberg 12.1
Development

Successfully merging this pull request may close these issues.

Provide suitable frontend fallback for the Navigation block when no Menu is selected
8 participants
@getdave @carolinan @bph @draganescu @adamziel @spacedmonkey @noisysocks @talldan